BUG: give each CustomSampler its own stream instead of the model's seed - #1102

Codecov Report

❌ Patch coverage is 97.22222% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 82.59%. Comparing base (e0ff281) to head (e775846).
⚠️ Report is 27 commits behind head on develop.

@@             Coverage Diff             @@
##           develop    #1102      +/-   ##
===========================================
+ Coverage    82.18%   82.59%   +0.41%     
===========================================
  Files          122      128       +6     
  Lines        16355    16583     +228     
===========================================
+ Hits         13441    13697     +256     
+ Misses        2914     2886      -28     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

Both of these were right and both are fixed in 4a1b6ac.

CRC32 collides. 32 bits is not enough for a stream domain, and a collision puts two samplers back on one stream, which is exactly what this PR exists to prevent. The reproducer holds:

crc32("wd4s4xka50") == crc32("p56cjcee10") == 1560575156
_sampler_seed(4242, ...) == 9598975395077455406   for both

My "107 built-in names, no collisions" was the wrong test. It says nothing about a subclass, a future field, or a name a user picks. The name is length-prefixed into spawn-key words now, which no two names can share, and the child keeps its full 128 bits instead of being cut to 64. That also lines it up with the width #1054 uses rather than having the two disagree.

The shared generator was order-coupled, and that one I introduced. Before this branch both wrappers got the same seed, so order did not matter. One seed per name made whichever wrapper reset last decide the stream:

declared x then y   (0.63119306, 0.61655487)
declared y then x   (0.61655487, 0.63119306)

Seeding is its own pass over sorted names now, so the shared generator lands on the same seed whichever way the model was written.

Where I did not follow the suggestion

You proposed a seed_group property on CustomSampler, with grouping and a per-group reset. I did the sorted pass instead, which is two lines against about forty and needs no change to the ABC or the documented wrappers.

It fixes what I broke. What it leaves is that two wrappers reading one generator take successive values, so swapping the declaration swaps which wrapper gets which number. That is inherent to sharing a generator rather than a seeding property, it is true on develop today, and the seed contract holds either way. If you would rather have the group abstraction anyway, say so and I will send it, but I would rather not add the API on my own initiative for a case that already behaves.

One thing worth flagging

My first attempt at the order fix was to sort the main validation loop, which looked like a one-word change. It is not: that loop's order sets __dict__, and dict_generator walks __dict__, so sorting it moved the draw order of every tuple in every model. A three-input model went from zulu, alpha, mike to alphabetical, with different values. The seeding pass is separate for that reason, and there is a test pinning it.

Local: ruff clean, pylint rocketpy/ tests/ docs/ exit 0, 2041 passed and 44 skipped. Three mutations, one per fix, each taking only its own test.

You were right and I was wrong to argue it away. 1defe34 adds the group semantics.

My case for the sorted pass was that it fixed what I had broken and the rest was inherent. It did fix the order coupling, but it left something I had also introduced, and I did not go looking for it:

shared group of wind_x, wind_y      x=0.63119306  y=0.61655487   generator reset 4x
add wind_z to the same generator    x=-0.6761184  y=0.49262482   generator reset 6x

Before this branch every sampler took the model's seed, so adding a member to a shared group changed nothing for the others. Keying by name protects independent samplers from exactly that, and left the shared group unprotected, because its stream was whatever the last-sorted member's seed made it. So the inconsistency was mine, not inherent.

CustomSampler now has a seed_group property, self by default, and a group is seeded once between its members with a seed derived from all their names. The documented wind wrappers return their shared generator. The generator is reset once rather than once per wrapper, which also stops the 1000-sample cache in that example being rebuilt and thrown away, so it takes a bite out of #1096 as well.

shared group                     reset 1x
independent sampler added        group unmoved
X.seed_group is Y.seed_group     True
correlation                      0.6986 against 0.6981

Adding a member to a group still moves that group's stream. That one does look inherent to me: the group is keyed by its members, and a different set of members is a different group. Say if you disagree.

One thing I did and then undid

Grouping keys on id, which is unique only among live objects, so I went looking for whether a seed_group that builds its answer could be freed and have its address reused. In a tight loop it certainly can: five successive temporaries all came back with the same id. I held a reference and wrote a test.

The test passed with the fix removed. In this loop's actual shape the temporaries do not collide, so the test proved nothing and I have taken it out. The reference is still held, because keying on an id you have not retained is fragile whether or not I could make it bite today, but the comment now says it is defensive rather than pretending it fixes something I observed.

Local: ruff clean, pylint rocketpy/ tests/ docs/ exit 0, 2043 passed and 44 skipped, and the documented example still executes out of the .rst.

Three more in 7fb0133, and the description is rewritten, since it still described the first version of this.

The documented example filled a cache it threw away. reset_seed generated 1000 pairs, and with per-index seeding that reset happens once per simulation, so a study built on this example produced a thousand pairs and used one, every time:

after construction       1000 pairs generated, none asked for
5 reseeds                5000 more generated, 0 used
                         0.099 ms each, about 10 s over 100k simulations

The cache starts empty now and top_up fills the shortfall on first use. Executed out of the .rst: 0 pairs after construction, 4000 after 4000 draws, correlation 0.6986 against the covariance's 0.6981.

The group reset went through a member. It dispatched to the first sampler in the group rather than to the group, which assumes every member resets identically and holds nothing of its own. The group owns the shared generator, so it is reset directly when it knows how, with the member as the fallback.

_sampler_seed sorts the names itself. The caller sorts today; a future one that forgets would hand a single group two different seeds.

I have also removed an earlier comment of mine here that said the conflict with #1054 was one line of imports. That was true of bffa1fac and stopped being true once this grew a seed_group contract and two signature changes, and it named a zlib import that no longer exists. The accurate version is in the description: the two overlap in the sampler lifecycle, they merge cleanly today, and I ran the merged tree rather than trusting that.

Local: ruff clean, pylint rocketpy/ tests/ docs/ exit 0, 2049 passed and 44 skipped, sixteen tests with a mutation each.

Pushed 74a6ba02 with the ownership rules on seed_group, which you asked for and which I had left implicit.

Two ways to break it, both silent, both measured:

Identity has to be stable across calls. Returning from a property makes it easy to build the answer each time, and then every member has a different identity and goes back into a group of its own. A two-member group drops from one reset to two:

stable identity, 2 members   -> group reset 1 time
unstable identity, 2 members -> group reset 2 times

A group belongs to one model. Declaring the same generator on two models has them both seed it and the later one wins, which is the overwrite the grouping is here to prevent.

I did not add tests for either. The first is a consequence of test_a_shared_group_is_seeded_once_between_its_members, which already fails if grouping stops working. The second is a caveat rather than a behaviour worth freezing, and a test asserting the last model wins would fail on anyone who later makes cross-model groups work properly.

I also considered detecting an unstable group and warning, and decided against it. It degrades to per-member seeding, which is the default anyway, so nothing goes wrong that would not have gone wrong without the property.

The wind pair in the docs returns a stored attribute, so the example already teaches the stable form.

Local: ruff clean, pylint 10.00/10 exit 0, stochastic unit suite 39 passed.

Added the changelog entry directly here, as you suggested, since the automation cannot run on a fork pull request until #1101 is fixed. Head is 10a0eeec.

It records both migrations: fixed-seed CustomSampler baselines move, and a sampler built on the legacy RandomState has to move to default_rng because the seed it now receives is 128 bits wide.

On the merge order, that matches what I found independently: #1104 first, then this branch updated so the Sphinx warnings-as-errors job actually runs against the custom_sampler.rst change. I will rebase once #1104 lands rather than now, so the run is against the real merge candidate.

On StochasticModel(seed=SeedSequence(...)): agreed it is not a blocker, and agreed the half-accepting behaviour is the part worth avoiding. The docstring says int, and _sampler_seed builds SeedSequence(entropy=seed, spawn_key=...), so a SeedSequence passed in would be rejected there while default_rng would have taken it. I would rather reject it early with a clear message than extend the state, since accepting it means deciding what a caller-supplied spawn_key composes with. Happy to add that guard here if you want it in scope, otherwise it belongs with the same question in #1054's root seed handling.

Merged into develop as 9593b1d, with your commits and authorship intact.

Done by hand rather than through the button, and worth saying why: the CHANGELOG.md entry conflicted with #1103, which landed a few minutes earlier. GitHub's Update branch refuses a conflicted head (422 merge conflict between base and head), and resolving it on my side means pushing to your fork, which I would rather not do uninvited. So the resolution — both entries kept, yours above #1103's — went in on the merge commit instead. Nothing else was touched: git diff against the previous develop is exactly your five files, 469 insertions.

What I checked before pushing:

• pytest tests/unit on the merge result — 1891 passed, 16 skipped
• The scoping question I had, since seeding moved out of the validation loop into its own pass: exception_list is initial_solution, terminate_on_apogee, date, ensemble_member, none of which can be a sampler, so _reset_custom_samplers walking the whole __stochastic_dict covers the same set the old loop did and no more.
• _sampler_seed keyed by sorted names, so the two-wrapper wind pair in the docs lands on one seed whichever order it is declared in — your test_a_shared_generator_lands_on_the_same_seed_whatever_the_order is the one that would have caught me if I had gotten this wrong.

The behaviour change is real and the changelog says so: fixed-seed CustomSampler baselines move, and a sampler built on the legacy RandomState now has to move to default_rng because the seed carries 128 bits. That is the right trade for two samplers no longer drawing the same deviate.

This PR will show as closed rather than merged, since the merge commit does not come from your branch. It is in develop regardless.

Correction, and the miss was mine.

I merged e7758461 — the head as of when I fetched your branch. You pushed 23be0bab sixteen seconds before my own push landed, and I did not re-check the head before closing. So the merge went in one commit short: develop had the assert isinstance(sampler, CustomSampler) you had just replaced, which is the exact python -O point I had raised on the review and you had already fixed.

Now applied as cbafd728, cherry-picked with your authorship intact. Content identical to 23be0bab; only the SHA differs.

Verified rather than assumed, since the whole point of that commit is a failure mode the plain test does not see:

• test_the_refusal_survives_python_dash_o against the old assert — fails, as it should. Against your raise — passes. It tests what it claims.
• pytest tests/unit — 1897 passed, 16 skipped. Six more than before, which is your two new tests plus the four the earlier merge had already brought in.
• ruff check and ruff format --check clean.

Two things worth saying plainly. Landing this by hand is what let a stale head through; the button would have refused, and that would have been the better outcome. And on the thirteen other asserts in stochastic_model.py — yes, send them separately. Same treatment, and there is a module-level TODO already asking for it.

Nothing was lost, and there was nothing wrong on your end.

Thanks for merging it by hand, and for saying why. The scoping check you did on exception_list is the one I would have wanted someone to make.

On your two review points, I went to fix the assert and found something that changes what is worth doing.

The one you flagged is inert. Both call sites guard the type immediately before calling:

elif isinstance(input_value, CustomSampler):
    attr_value = self._validate_custom_sampler(input_name, input_value)   # line 141
...
elif isinstance(input_value, CustomSampler):
    return self._validate_custom_sampler(input_name, input_value)         # line 358

So it can only fire if the private method is called directly, which is what my own test was doing. Under python -O it disappears and nothing changes. I had told myself it was the last thing keeping a non-sampler out of the model; that was wrong, and I only noticed when I went looking for what it protected.

Thirteen others in the same file are not inert. They validate what the user passes in, and they do vanish:

StochasticModel(obj, mass=("not a number", 0.5))

  python      AssertionError
  python -O   accepted, mass = ('not a number', 0.5, <bound method Generator.normal>)

That is a malformed model that will fail somewhere much later with something unhelpful. rocketpy/ has 22 of these, all in stochastic/: 14 in stochastic_model.py, 3 each in stochastic_environment.py and stochastic_flight.py, 1 in stochastic_aero_surfaces.py, and 1 left in stochastic_parachute.py that #1103 did not reach.

Happy to send all 22 as one pull request, keeping AssertionError so nothing that catches it today breaks. That seems more useful than converting the one that cannot fire. Say if you would rather have it split per file, or scoped to stochastic_model.py first.

On the broad handler. That one I would defend, and I want to give you the reason rather than just leave it. It catches to name the sampler and re-raises with from error, so nothing is swallowed and the original stays as __cause__. It was RuntimeError before, and the reason for widening is in the tests: a sampler built on the legacy RandomState raises ValueError on a 128-bit seed, and a bare one of those says nothing about which of the model's inputs produced it.

If the convention is firm regardless, the narrow version is except (RuntimeError, ValueError, TypeError), which covers the cases I know about and loses the name for anything else a user's reset_seed does. I will send whichever you prefer.